Added support for compiling TLS into binaries #508

AlvinMooreSr · 2018-06-20T16:49:09Z

Resolves issue #436

alexmiller-apple · 2018-06-20T19:13:44Z

fdbcli/fdbcli.actor.cpp

 	TraceEvent::setNetworkThread();
+	printf("Set network thread\n");


These are still here

alexmiller-apple · 2018-06-20T19:20:58Z

fdbrpc/md5/md5.h

-	void MD5_Init(MD5_CTX *ctx);
-	void MD5_Update(MD5_CTX *ctx, const void *data, unsigned long size);
-	void MD5_Final(unsigned char *result, MD5_CTX *ctx);
+	void __attribute__((weak)) MD5_Init(MD5_CTX *ctx);


I think you'll need to add a preprocessor macro, probably to flow/Platform.h, which looks like

#ifndef MULTIPLY_DEFINED_SYMBOL #if windows #define MULTIPLY_DEFINED_SYMBOL __declspec(selectany) #else #define MULTIPLY_DEFINED_SYMBOL __attribute__((weak)) #endif #endif

and then use MULTIPLY_DEFINED_SYMBOL, or some better name, here, as msvc doesn't support attribute((weak)).

alexmiller-apple · 2018-06-20T19:23:18Z

bindings/c/local.mk

@@ -26,6 +26,10 @@ fdb_c_LIBS := lib/libfdbclient.a lib/libfdbrpc.a lib/libflow.a
 fdb_c_tests_LIBS := -Llib -lfdb_c
 fdb_c_tests_HEADERS := -Ibindings/c

+ifndef __TLS_DISABLED__


I'd much prefer TLS_DISABLED for the Makefile and #define. Two leading underscore names are technically reserved by the C/C++ standard.

alexmiller-apple · 2018-06-20T19:27:59Z

bindings/c/local.mk

@@ -26,6 +26,10 @@ fdb_c_LIBS := lib/libfdbclient.a lib/libfdbrpc.a lib/libflow.a
 fdb_c_tests_LIBS := -Llib -lfdb_c
 fdb_c_tests_HEADERS := -Ibindings/c

+ifndef __TLS_DISABLED__
+fdb_c_LIBS += lib/libFDBLibTLS.a /usr/local/lib/libtls.a /usr/local/lib/libssl.a /usr/local/lib/libcrypto.a


I think it'd be better do the same $(CC) $(CFLAGS) -print-file-name libtls.a sort of trick for each static library, and assign that to some ${TLS_LIBRARIES} variable, so that we don't have to pin the libraries to a specific file path.

And having a TLS_LIBRARIES variable that holds the above list of libraries if it is enabled and nothing if it's disabled lets you remove all the other ifndef __TLS_DISABLED__'s from the Makefile.

Was there a reason you decided not to go the -Bstatic -l tls -l ssl -l crypto -Bdynamic route? That'd make the above explicit file path naming unnecessary.

alexmiller-apple · 2018-06-20T19:29:55Z

FDBLibTLS/Makefile

@@ -1,109 +0,0 @@
-PROJECTPATH = $(dir $(realpath $(firstword $(MAKEFILE_LIST))))


Should we at least move test, test-plugin, and test-verify to somewhere else in our build system so that we can do development on the plugin code?

Or has statically-linking the plugin meant that this testing framework no longer works?

Given that I (and previously you) were just asked to do a few enhancements to this code, I'm more nervous than before about us deprecating testing that seems important to keep.

alexmiller-apple · 2018-06-20T19:36:04Z

fdbrpc/LoadPlugin.h

@@ -20,13 +20,14 @@

 #pragma once

+// Will need to be defined at link time
+extern "C" void *get_plugin(const char *plugin_type_name_and_version);


I'd think we should probably rename the TLS plugin's get_plugin to something TLS-specific

alexmiller-apple · 2018-06-20T19:38:21Z

fdbrpc/LoadPlugin.h

-	else
-		return Reference<T>( NULL );
+#ifdef __TLS_DISABLED__
+	return Reference<T>( NULL );


and check plugin_name == fdb-libressl-plugin, or something, and then call get_tls_plugin if it matches and get_plugin if it doesn't.

We should either leave the plugin interface as a generic working plugin interface, and make it special case out the TLS plugin that's now built-in, or just remove the generic plugin interface entirely. But I'm kind of against just crippling it whenever we aren't using the TLS code.

what about this comment?

alexmiller-apple · 2018-06-20T19:39:11Z

fdbrpc/TLSConnection.actor.cpp

-void TLSOptions::init_plugin( std::string const& plugin_path ) {
-	std::string path;
+void TLSOptions::init_plugin() {
+	std::string plugin_path = platform::getDefaultPluginPath("fdb-libressl-plugin");


There's not much of a point in expanding this out to a filesystem path that will never again exist, right? It seems easier to just leave this as a fake path of just "fdb-libressl-plugin", so that you have something fixed to match against later on.

Disable TLS within Windows until working

alexmiller-apple · 2018-06-26T21:22:05Z

build/link-wrapper.sh

@@ -19,6 +19,7 @@ case $1 in
 	fi

 	OPTIONS=$( eval echo "$OPTIONS $LDFLAGS \$$2_LDFLAGS \$$2_OBJECTS \$$2_LIBS \$$2_STATIC_LIBS_REAL -o $3" )
+	echo "OPTIONS: $OPTIONS"


alexmiller-apple · 2018-06-26T21:22:10Z

build/link-wrapper.sh

@@ -33,6 +34,7 @@ case $1 in
 		fi
 		;;
 	    *)
+		echo "Command: $CC $OPTIONS"


alexmiller-apple · 2018-06-26T21:23:03Z

fdbrpc/LoadPlugin.h

-	else
-		return Reference<T>( NULL );
+#ifdef __TLS_DISABLED__
+	return Reference<T>( NULL );


what about this comment?

alexmiller-apple · 2018-06-26T21:24:27Z

fdbcli/fdbcli.actor.cpp

 	TraceEvent::setNetworkThread();
+	printf("Set network thread\n");


These are still here

…r TLS use case Defined TLS plugin name constant Changed TLS plugin name to get_tls_plugin Fixed link script Removed compilation flags from info make target

Changed TLS documentation to not specify it as a plugin but compiled in

alexmiller-apple · 2018-06-27T21:26:45Z